Restore BitArray constructor performance - #131833
Draft
joshuajyue wants to merge 1 commit into
Draft
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4236475d-0243-488d-93d8-cd78e94d532b
Member
Author
|
@EgorBot -windows_x64 -attempts 3 --filter "Perf_BitArray.BitArrayBoolArrayCtor" Note This benchmark request was generated with GitHub Copilot. |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tweaks BitArray constructors to reduce JIT impediments in hot paths, with the goal of restoring constructor microbenchmark performance after recent refactoring.
Changes:
- Avoids using an
out-parameter as the source value for allocation sizing in the byte/int span helpers by computing the bit length in a local first. - Removes the
out bitLengthparameter from the Boolean packing helper and assigns_bitLengthdirectly from the source length in the constructors. - Switches the Boolean packing loop induction variable to
intto keep span indexing and bounds analysis in canonical signed form for the JIT.
Suppressed comments (1)
src/libraries/System.Private.CoreLib/src/System/Collections/BitArray.cs:342
lengthhere is a bit count (values.Length * BitsPerInt32), but the name reads like an element/byte count. Consider renaming it tobitLengthLocal(and updating the two uses) to make the unit explicit.
int length = values.Length * BitsPerInt32;
byte[] array = AllocateByteArray(length);
if (BitConverter.IsLittleEndian)
{
Comment on lines
+175
to
+179
| int length = bytes.Length * BitsPerByte; | ||
| byte[] array = AllocateByteArray(length); | ||
|
|
||
| bytes.CopyTo(array); | ||
| bitLength = length; |
Member
Author
|
@EgorBot -windows_x64 -attempts 3 --filter "Perf_BitArray.BitArrayIntArrayCtor" --filter "Perf_BitArray.BitArraySetLengthGrow" Note This benchmark request was generated with GitHub Copilot. |
Member
|
I've filed an alternative fix to make it fully memory safe #131838 (part of the on-going effort). Let's see if it improves. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
outvalues, removing the Boolean helper'soutparameter entirely and delaying byte and integeroutassignments until after allocation and copying.intinduction variable in the Boolean SIMD and scalar remainder paths so span indexing and bounds analysis stay in the JIT's canonical signed form.Fixes #131815
Performance
Performance tracking identified regressions in three affected benchmark groups:
BitArrayBoolArrayCtor: automated bisection tied the confirmedSize: 512regression to Add ReadOnlySpan constructors to BitArray #131500 (16.73 ns to 22.65 ns, approximately +35%).BitArrayIntArrayCtor: additional regressions were reported at multiple input sizes and are being validated with a focused Windows x64 EgorBot run.BitArraySetLengthGrow(Size: 4): this benchmark includes construction frombyte[]. Automated bisection instead pointed to unrelated JIT commit0bbeda6cbad4and flagged possible code-alignment noise, so a focused EgorBot run is validating whether this fix affects it.A local same-testhost ARM64 paired run of the merged Boolean implementation versus this fix (50 alternating AB/BA pairs) found no statistically significant difference: 37.24 ns versus 37.96 ns, p=0.426. Focused Windows x64 EgorBot runs will validate the original regression environment.
Validation
System.Private.CoreLibbuildBitArray_CtorTestspassed with hardware intrinsics enabledBitArray_CtorTestspassed withDOTNET_EnableHWIntrinsic=0Note
This pull request description was generated with GitHub Copilot and reviewed before publication.